Conversation
There was a problem hiding this comment.
Pull request overview
Adds a baseline golangci-lint configuration and fixes a couple of staticcheck findings, while also updating Go/module metadata and dependencies.
Changes:
- Add
.golangci.ymlenabling a limited set of linters. - Fix an invalid regexp flag in
ts/commands.goand simplify anfmt.Errorf(fmt.Sprintf(...))pattern inops/downloads.go. - Update
go.mod/go.sum(Go version + multiple dependency bumps) and remove a Go build constraint indownloads/tarball_registry.go.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.golangci.yml |
Introduces golangci-lint configuration (enabled linters + issue limits). |
ts/commands.go |
Adjusts regexp to avoid invalid syntax flagged by staticcheck. |
ops/downloads.go |
Simplifies error construction flagged by staticcheck. |
go.mod |
Raises Go version directive and upgrades multiple dependencies. |
go.sum |
Updates checksums consistent with dependency version changes. |
downloads/tarball_registry.go |
Removes go1.16 build tags from the embedded registry source file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reLines := regexp.MustCompile(`(?s)(^.*ERROR.*)`) | ||
| errorLines := reLines.FindAll(contents, -1) | ||
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, errorLines) |
There was a problem hiding this comment.
The regex used to collect lines with "ERROR" still won’t return individual log lines: with (?s)(^.*ERROR.*) and no multiline flag, ^ anchors to the start of the whole file, so FindAll will typically produce a single large match (potentially spanning multiple lines). Consider switching to a multiline pattern (e.g., using (?m) with line anchors) or line-based scanning so you report the specific lines that contain ERROR.
| reLines := regexp.MustCompile(`(?s)(^.*ERROR.*)`) | |
| errorLines := reLines.FindAll(contents, -1) | |
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, errorLines) | |
| reLines := regexp.MustCompile(`(?m)^.*ERROR.*$`) | |
| errorLines := reLines.FindAllString(string(contents), -1) | |
| ts.Fatalf("ERRORs found in %s\n%s\n", logFile, strings.Join(errorLines, "\n")) |
| reLines := regexp.MustCompile(`(?sg)(^.*ERROR.*)`) | ||
| reLines := regexp.MustCompile(`(?s)(^.*ERROR.*)`) | ||
| errorLines := reLines.FindAll(contents, -1) | ||
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, errorLines) |
There was a problem hiding this comment.
errorLines is a slice of byte-slices ([][]byte), but it’s formatted with %s in ts.Fatalf. This will produce a formatting error (e.g., %!s([][]uint8=...)) instead of the actual lines. Convert/join the matches into a string (or format each line) before passing to %s.
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, errorLines) | |
| var errorLinesStr []string | |
| for _, line := range errorLines { | |
| errorLinesStr = append(errorLinesStr, string(line)) | |
| } | |
| ts.Fatalf("ERRORs found in %s (%s)\n", logFile, strings.Join(errorLinesStr, "; ")) |
| linters: | ||
| enable: | ||
| - errcheck | ||
| - govet | ||
| - ineffassign | ||
| - staticcheck | ||
| - unused |
There was a problem hiding this comment.
This config enables a few linters, but without linters.disable-all: true golangci-lint will also run its default-enabled linters (which can change between golangci-lint versions). If the intent is to run only the listed conservative set, add disable-all: true and keep the explicit enable list.
| module github.com/datacharmer/dbdeployer | ||
|
|
||
| go 1.18 | ||
| go 1.22 |
There was a problem hiding this comment.
Bumping the module go directive from 1.18 to 1.22 is a breaking change in supported toolchains and goes beyond the PR goal of adding lint config / fixing lint bugs. If 1.22 is required, please update the documented build requirements and ensure CI/tooling expectations match; otherwise consider keeping the existing Go version for this PR.
| github.com/rogpeppe/go-internal v1.12.0 | ||
| github.com/spf13/cobra v1.8.1 | ||
| github.com/spf13/pflag v1.0.6 | ||
| github.com/stretchr/testify v1.9.0 |
There was a problem hiding this comment.
This PR upgrades a number of direct dependencies (cobra, mysql driver, testify, x/*, etc.). That’s a much broader behavioral surface-area than “fix critical lint issues only” and makes it harder to attribute regressions to the lint-related changes. Consider reverting the dependency bumps here (or splitting them into a dedicated dependency-upgrade PR with its own testing/validation).
| github.com/stretchr/testify v1.9.0 | |
| github.com/stretchr/testify v1.8.4 |
| if err != nil { | ||
| return downloads.TarballDescription{}, fmt.Errorf(fmt.Sprintf("Error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %s", | ||
| version, flavor, OS, minimal, newest, guessLatest, err)) | ||
| return downloads.TarballDescription{}, fmt.Errorf("Error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %s", |
There was a problem hiding this comment.
The returned error string is capitalized ("Error getting version...") and the original error is interpolated with %s, which loses error wrapping. Prefer a lower-case error message and wrap the underlying error (so callers can use errors.Is/As).
| return downloads.TarballDescription{}, fmt.Errorf("Error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %s", | |
| return downloads.TarballDescription{}, fmt.Errorf("error getting version %s (%s-%s)[minimal: %v - newest: %v - guess: %v]: %w", |
|
@gemini-code-assist review |
|
@claude review |
There was a problem hiding this comment.
Code Review
This pull request introduces golangci-lint to the project, a great step for improving code quality. It also includes updates to the Go version and dependencies, along with fixes for issues identified by the new linter. The changes are solid. I've added one suggestion in ts/commands.go to improve the error reporting in a test helper, making test failures more informative.
| hasError := strings.Contains(string(contents), "ERROR") | ||
| if neg && hasError { | ||
| reLines := regexp.MustCompile(`(?sg)(^.*ERROR.*)`) | ||
| reLines := regexp.MustCompile(`(?s)(^.*ERROR.*)`) |
There was a problem hiding this comment.
While your change correctly fixes the invalid regex flag, the regex (?s)(^.*ERROR.*) will match the entire file content as a single block if an 'ERROR' is present. This can make the test failure output very large and hard to read. To find and report only the specific lines containing 'ERROR', you could use a multi-line regex. This will make the test output more focused and useful.
Note that with this change, the output will be a list of lines containing errors, which is more informative. However, for even better formatting, you might consider converting the resulting byte slices to strings and joining them with newlines in the ts.Fatalf call in a follow-up change.
| reLines := regexp.MustCompile(`(?s)(^.*ERROR.*)`) | |
| reLines := regexp.MustCompile(`(?m)^.*ERROR.*$`) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add a conservative golangci-lint config enabling errcheck, govet, ineffassign, staticcheck, and unused linters. Fix two real bugs caught by staticcheck: - SA1000: invalid regex syntax (?sg) in ts/commands.go (Go doesn't support 'g' flag) - SA1006: fmt.Errorf wrapping fmt.Sprintf unnecessarily in ops/downloads.go
147b025 to
74e3748
Compare
Summary
.golangci.ymlconfiguration enabling errcheck, govet, ineffassign, staticcheck, and unused linters(?sg)->(?s)) ints/commands.gocaught by staticcheck SA1000fmt.Errorf(fmt.Sprintf(...))pattern inops/downloads.gocaught by staticcheck SA1006Closes #2
Test plan
golangci-lint run ./...completes without critical errorsgo test ./...)ts/commands.godoesn't change test behavior (removed invalidgflag that Go regexp never supported)